Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

src/openhuman/approval/gate.rs (2)

1400-1479: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Solid regression coverage for the external-abort cleanup path.

Both tests correctly validate waiter eviction, routing-map cleanup, and durable Deny persistence for chat- and meeting-routed parks. One gap worth considering: there's no test exercising the race this design is built to survive — decide() committing an Approve* decision concurrently with the abort, verifying the guard's store::decide(Deny) becomes a no-op (per the WHERE decided_at IS NULL conditional) rather than clobbering the already-persisted decision. That's the core correctness guarantee cited in the PR objectives ("conditional updates preserve concurrently committed decisions") and isn't directly covered here.

Want me to draft a test that calls gate.decide(&request_id, ApprovalDecision::ApproveOnce) immediately before handle.abort() and asserts the persisted decision stays ApproveOnce?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/approval/gate.rs` around lines 1400 - 1479, Add regression
coverage for the abort/decision race in the existing external-abort waiter
tests: commit an approval via gate.decide using the captured request_id
immediately before aborting the spawned waiter, then assert cleanup still occurs
and store::get_decision remains ApprovalDecision::ApproveOnce. Ensure the test
exercises the conditional Deny cleanup path so it cannot overwrite an
already-persisted approval.

---

206-271: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Blocking SQLite write inside Drop::drop can stall the runtime thread performing the cancellation.

Drop::drop (lines 237-270) calls store::decide(&self.gate.config, &self.request_id, ApprovalDecision::Deny) synchronously (line 254). Drop cannot yield, so whichever thread runs this destructor — plausibly a Tokio worker thread reclaiming the task during abort() or an outer-deadline drop — is blocked for the duration of the SQLite write/lock acquisition. Tokio's own docs call this out explicitly: "if you call a non-async method from async code, that non-async method is still inside the asynchronous context, so you should also avoid blocking operations there. This includes destructors of objects destroyed in async code."

This mirrors the existing synchronous store::decide calls at lines 842/862, but doing it in Drop is strictly worse: there's no way to bound it with a timeout or defer it, and it fires precisely during the failure scenario this PR targets (turn-deadline cancellation), which is exactly when many tasks might be torn down in a burst.

Consider offloading via tokio::task::spawn_blocking (fire-and-forget, logging errors from the spawned task) so the drop path never blocks its calling thread — or explicitly document that local SQLite writes are treated as acceptable "fast enough" blocking work here.

♻️ Possible direction (fire-and-forget via spawn_blocking)

         match store::decide(&self.gate.config, &self.request_id, ApprovalDecision::Deny) {
-            Ok(Some(_)) => tracing::debug!(...),
-            Ok(None) => tracing::debug!(...),
-            Err(err) => tracing::error!(...),
-        }
+        }
+        let config = self.gate.config.clone();
+        let request_id = self.request_id.clone();
+        tokio::task::spawn_blocking(move || {
+            match store::decide(&config, &request_id, ApprovalDecision::Deny) {
+                Ok(Some(_)) => tracing::debug!(request_id = %request_id, "..."),
+                Ok(None) => tracing::debug!(request_id = %request_id, "..."),
+                Err(err) => tracing::error!(request_id = %request_id, error = %err, "..."),
+            }
+        });

Note Config must be cheaply cloneable (or wrapped in Arc) for this to work without extra cost — worth confirming before adopting.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/approval/gate.rs` around lines 206 - 271, Remove the
synchronous store::decide call from ParkedApprovalCleanupGuard::drop so
destructor execution never performs SQLite I/O. Preserve the existing in-memory
cleanup and denial behavior by cloning or otherwise safely capturing the
required config and request_id, then dispatching the denial persistence through
tokio::task::spawn_blocking with equivalent success and error logging inside the
spawned task; handle any task-spawn failure without blocking Drop.